Reorder filters and let RealmContextResolver throw UnresolvableRealmContextException by adutra · Pull Request #848 · apache/polaris

adutra · 2025-01-22T17:14:10Z

Fixes #819.

Summary of changes:

Vert.x route filters were converted to JAX-RS ContainerRequestFilter
New RealmIdFilter introduced
RealmIdResolver now can throw UnresolvableRealmException => maps to HTTP 404
Added tests in PolarisApplicationIntegrationTest
Fixed default realm name in tests
Priorities were revisited, new order of execution is shown below:

Filter	Order	Priority
`RealmIdFilter`	1	900
`QuarkusLoggingMDCFilter`	2	901
`QuarkusTracingFilter`	3	902
`PolarisPrincipalAuthenticatorFilter`	4	1000
`PolarisPrincipalRolesProviderFilter`	5	1001
`RateLimiterFilter`	6	5000

adutra · 2025-01-24T10:31:25Z

This is now ready for review.

dimas-b · 2025-01-24T21:22:29Z

integration-tests/src/main/java/org/apache/polaris/service/it/env/PolarisApiEndpoints.java

Would you mind moving this to a (new) method on PolarisServerManager or Server so that it is possible to customize?

dimas-b · 2025-01-24T21:25:50Z

...ests/src/main/java/org/apache/polaris/service/it/test/PolarisApplicationIntegrationTest.java

nit: maybe make internalCatalogName a parameter to this test?

dimas-b · 2025-01-24T21:35:01Z

service/common/src/main/java/org/apache/polaris/service/context/RealmIdFilter.java

I'm not sure about 401 in this context. I believe it generally means your credentials are not valid, which is not applicable to failures in realm resolution.

I'm thinking that a 404 may fit better. For example, GH gives 404 on accessing repositories (i.e. realms) to which the user has no access.

Makes sense, i will update the PR title and description accordingly.

quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusProducers.java

dimas-b · 2025-01-24T21:44:08Z

service/common/src/main/java/org/apache/polaris/service/context/DefaultRealmIdResolver.java

I think it could be valuable to cover this with an explicit config flag. If realms are used in a particular deployment, it is probably not wise to allow potentially malicious clients to try sending requests at the default realm without knowing its ID.

I completely agree, I think that having a default realm is a potential security issue.

dimas-b

Nice improvement to request filters! Thanks, @adutra !

As far as I can tell this change is backward compatible at the REST API / HTTP level.

snazy · 2025-01-27T17:43:00Z

integration-tests/src/main/java/org/apache/polaris/service/it/env/PolarisApiEndpoints.java

Maybe rename to realmHeaderName() + realmId() (latter for consistency w/ Server)?

snazy · 2025-01-27T17:44:27Z

integration-tests/src/main/java/org/apache/polaris/service/it/env/Server.java

Suggested change

default String realmHeader() {

default String realmHeaderName() {

snazy · 2025-01-27T17:50:10Z

service/common/src/main/java/org/apache/polaris/service/context/RealmContextConfiguration.java

Suggested change

* <p>Note: this is actually only enforced when using {@link DefaultRealmIdResolver}. When using

* {@link TestRealmIdResolver}, this setting is ignored.

* <p>Note: this setting is only enforced in production setups.

snazy · 2025-01-27T17:50:31Z

service/common/src/main/java/org/apache/polaris/service/context/RealmContextConfiguration.java

Should this be true by default?

Defaulting to true would be a behaviour change at the REST/HTTP level. I'd prefer to keep old behaviour in this PR and change defaults later.

Yeah changing this to true would break downstreams. But we can mention this in the "Configuring for production" docs.

snazy · 2025-01-27T17:53:03Z

service/common/src/main/java/org/apache/polaris/service/context/RealmIdFilter.java

Wonder if it makes more sense to change RealmIdResolver to expect a Function<String, String> rather than a "full blown map" (and use request::getHeader here).

dimas-b · 2025-01-28T03:59:10Z

@collado-mike : any concerns from your POV?

adutra · 2025-02-04T10:34:37Z

PR Rebased. @dimas-b @snazy @collado-mike can I get another review please?

dimas-b

LGTM 👍 I found a fresh, but tangential issue, however I think we can address it later (if people agree with my proposal below).

dimas-b · 2025-02-04T22:36:23Z

...s/service/src/main/java/org/apache/polaris/service/quarkus/tracing/QuarkusTracingFilter.java

tangential: When OTel is disabled, it's still ok to operate on Span.current(), we'd simply get a "no-op" Span. However, IMHO it is preferable to exercise the code that produces span values even when tracing is disabled. This is to avoid bugs that only pop up when tracing is on.

I get the sentiment, but at the same time, it's nice to be able to turn off the functionality entirely, including any bugs that may be present that could impact the running service even though you don't want/care about tracing.

…ontextException

adutra · 2025-02-05T18:23:20Z

This has been in review for 2 weeks. Is it OK to merge now? @collado-mike

collado-mike

LGTM

collado-mike · 2025-02-05T23:02:42Z

...s/service/src/main/java/org/apache/polaris/service/quarkus/tracing/QuarkusTracingFilter.java

I get the sentiment, but at the same time, it's nice to be able to turn off the functionality entirely, including any bugs that may be present that could impact the running service even though you don't want/care about tracing.

collado-mike · 2025-02-05T23:06:49Z

...ice/common/src/main/java/org/apache/polaris/service/context/DefaultRealmContextResolver.java

      if (!configuration.realms().contains(realm)) {
-        throw new IllegalArgumentException("Unknown realm: " + realm);
+        throw new UnresolvableRealmContextException("Unknown realm: " + realm);
      }


Personally, i would push the check down into the RealmContextConfiguration rather than exposing a set of valid realms and doing the check here. But for this change, I think the UnresolvableRealmContextExcpetion is the right change to make

adutra force-pushed the filter-order branch 2 times, most recently from e542f5c to 9c45494 Compare January 22, 2025 17:50

adutra mentioned this pull request Jan 23, 2025

Rethink order of JAX-RS filters in Polaris #819

Closed

adutra force-pushed the filter-order branch from 9c45494 to 73ef947 Compare January 24, 2025 10:28

adutra changed the title ~~[WIP] Reorder filters and let RealmIdResolver throw NotAuthorized~~ Reorder filters and let RealmIdResolver throw NotAuthorized Jan 24, 2025

adutra marked this pull request as ready for review January 24, 2025 10:29

adutra requested review from RussellSpitzer, ashvina, collado-mike, dennishuo, dimas-b, ebyhr, eric-maynard, flyrain, jackye1995, jbonofre, snazy, takidau and vvcephei as code owners January 24, 2025 10:29

dimas-b reviewed Jan 24, 2025

View reviewed changes

adutra force-pushed the filter-order branch from 73ef947 to 9e932b2 Compare January 27, 2025 13:32

adutra changed the title ~~Reorder filters and let RealmIdResolver throw NotAuthorized~~ Reorder filters and let RealmIdResolver throw UnresolvableRealmException Jan 27, 2025

dimas-b approved these changes Jan 27, 2025

View reviewed changes

snazy reviewed Jan 27, 2025

View reviewed changes

adutra requested a review from MonkeyCanCode as a code owner January 27, 2025 17:58

snazy approved these changes Jan 27, 2025

View reviewed changes

dimas-b approved these changes Jan 27, 2025

View reviewed changes

adutra force-pushed the filter-order branch from b3daca9 to f062c52 Compare February 4, 2025 10:33

adutra changed the title ~~Reorder filters and let RealmIdResolver throw UnresolvableRealmException~~ Reorder filters and let RealmContextResolver throw UnresolvableRealmContextException Feb 4, 2025

dimas-b approved these changes Feb 4, 2025

View reviewed changes

Reorder filters and let RealmContextResolver throw UnresolvableRealmC…

ad666e5

…ontextException

adutra force-pushed the filter-order branch from f062c52 to ad666e5 Compare February 5, 2025 08:16

collado-mike approved these changes Feb 5, 2025

View reviewed changes

dimas-b merged commit 1f3023d into apache:main Feb 6, 2025
5 checks passed

adutra deleted the filter-order branch February 6, 2025 11:27

	default String realmHeader() {
	default String realmHeaderName() {

	* <p>Note: this is actually only enforced when using {@link DefaultRealmIdResolver}. When using
	* {@link TestRealmIdResolver}, this setting is ignored.
	* <p>Note: this setting is only enforced in production setups.

Conversation

adutra commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adutra commented Jan 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b commented Jan 28, 2025

Uh oh!

adutra commented Feb 4, 2025

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adutra commented Feb 5, 2025

Uh oh!

collado-mike left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adutra commented Jan 22, 2025 •

edited

Loading